New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TekSavvy Sensor unlimited bandwidth support #12325
TekSavvy Sensor unlimited bandwidth support #12325
Conversation
'usage_gb': ['Usage', GIGABYTES, 'mdi:download'], | ||
'limit': ['Data limit', GIGABYTES, 'mdi:download'], | ||
'onpeak_download': ['On Peak Download', GIGABYTES, 'mdi:download'], | ||
'onpeak_upload': ['On Peak Upload ', GIGABYTES, 'mdi:upload'], | ||
'onpeak_upload': ['On Peak Upload', GIGABYTES, 'mdi:upload'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and line 35 would be breaking changes?
How do I ensure that makes it into the release notes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notably sensor.teksavvy_on_peak_upload_
(which is incorrect with the trailing underscore) becomes sensor.teksavvy_on_peak_upload
Additionally with both the percentage and GB values both being named usage
you would get a non-deterministic result in tests for sensor.teksavvy_usage
and sensor.teksavvy_usage_2
.
Renaming to Usage Ratio
makes it very deterministic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a section to your PR description and I'll add the label "Breaking Change"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a description, thanks!
Test seems to be failing on |
@@ -18,6 +18,11 @@ | |||
import homeassistant.helpers.config_validation as cv | |||
from homeassistant.helpers.entity import Entity | |||
from homeassistant.util import Throttle | |||
# Python 3.4 support | |||
try: | |||
from json.decoder import JSONDecodeError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ValueError. JSONDecodeError is new in Python 3.4.
(or wait 2 weeks and we drop Py34 support)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped to ValueError
only.
self.data["offpeak_total"] = off_peak_download + off_peak_upload | ||
self.data["onpeak_remaining"] = limit - on_peak_download | ||
return True | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I see it. Will update.
off_peak_upload | ||
self.data["onpeak_remaining"] = limit - on_peak_download | ||
return True | ||
except ValueError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What can cause a JSON decode error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an edge case, assuming the API responds with HTTP OK/200 but the payload data isn't proper JSON. Then this would catch it.
off_peak_upload = self.data["offpeak_upload"] | ||
limit = self.data["limit"] | ||
# Support "unlimited" users | ||
self.data["usage"] = 100*on_peak_download/self.bandwidth_cap\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rewrite as normal if/else if the inline if/else has to span multiple lines.
if self.bandwidth_cap > 0 else 0 | ||
self.data["usage_gb"] = on_peak_download | ||
self.data["onpeak_total"] = on_peak_download + on_peak_upload | ||
self.data["offpeak_total"] = off_peak_download +\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put the \
after the =
?
|
||
|
||
@asyncio.coroutine | ||
def test_bad_json_decond(hass, aioclient_mock): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix, oops
Support TekSavvy account usage for unlimited plans. Seeing cap limit to 0 will now provide unlimited behaviour on usage calculations.
Add coverage unit tests to TekSavvy Sensor component, none existing previously.
Poke for updated review. |
The documentation update seems merged and live for 0.64, but this not being merged makes it but match. Not a huge deal, but if a user sets 0 for unlimited they would recieve a division by zero exception. |
Description:
Add support for ISP TekSavvy sensor on uncapped plans.
Unit tests added for TekSavvy component, none were present previously.
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4645
Breaking Change:
The sensor entity id for peak upload usage used to be
sensor.teksavvy_on_peak_upload_
this has been changed tosensor.teksavvy_on_peak_upload
The
usage
title was shared between and therefore indeterminate between GB and % usage. Therefore % usage entity ID has been changed tosensor.teksavvy_usage_ratio
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass